Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

quotapool,kvserver: extend quotapool.RateLimit, rate limit queue addition in SystemConfigUpdate #53605

Merged
merged 4 commits into from
Aug 31, 2020
Merged

quotapool,kvserver: extend quotapool.RateLimit, rate limit queue addition in SystemConfigUpdate #53605

merged 4 commits into from
Aug 31, 2020

Conversation

ajwerner
Copy link
Contributor

See individual commits.

This PR is a fancier implementation of #53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner mentioned this pull request Aug 28, 2020
Merged
@ajwerner ajwerner force-pushed the ajwerner/dont-add-to-queues-on-every-single-system-config-update branch from 6cfdfc4 to 1898ec9 Compare August 28, 2020 22:25
@ajwerner
quotapool: add ability to update configuration of the IntRatePool
4240a92 
Release justification: non-production code changes
Release note: None
@ajwerner
quotapool: add AdmitN a non-blocking call to the RateLimiter
c935d5f 
Release justification: low risk, high benefit changes to existing functionality
Release note: None
@ajwerner
quotapool: export the RateLimiter
7823fc8 
Release justification: non-production code changes

Release note: None
@ajwerner
kvserver: limit the frequency at which system config updates enqueue …
ab3f53d 
…replicas

This has a shockingly profound impact on the runtime of ORM tests.

This PR is a fancier implementation of cockroachdb#53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.
@ajwerner ajwerner force-pushed the ajwerner/dont-add-to-queues-on-every-single-system-config-update branch from 1898ec9 to ab3f53d Compare August 31, 2020 02:43
@ajwerner ajwerner mentioned this pull request Aug 31, 2020
Closed
@ajwerner ajwerner requested a review from tbg August 31, 2020 02:44
knz
knz reviewed Aug 31, 2020
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just reviewed this and I feel rather comfortable by the functional correctness of the thing (i.e. I can't see it blocking unexpectedly)

Can you speak a bit more quantitatively about what motivated this change and the visible benefits?

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

knz
knz reviewed Aug 31, 2020
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

@ajwerner
Copy link
Contributor Author

Prior to this change, over 70% of the CPU utilization of the typeorm roachtest was underneath those calls. The change decreases the runtime of the typeorm by 30% (after applying #53589, have not measured the impact of this change against master but I'd suspect it to be even larger as that PR reduces the amount of work done to run jobs). I'm rerunning on top of master and against master to compare those runtime and CPU differences.

image

Before:

--- PASS: typeorm (1444.19s)
--- PASS: typeorm (1463.58s)
--- PASS: typeorm (1530.58s)

After

--- PASS: typeorm (1094.87s)
--- PASS: typeorm (1094.73s)
--- PASS: typeorm (1084.94s)

@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 31, 2020 •
edited by knz
Loading

According to a 30s profile from before and after, this change reduces the CPU utilization due to the calls to adding to the queue from 69.7% to 4.9% and reduces the CPU utilization overall dramatically. I'll post runtime differences when they finish. See the below charts:

Before:
image

After:
image

@knz
Copy link
Contributor

knz commented Aug 31, 2020

this looks gorgeous!

I'm LGTM on this

@ajwerner
Copy link
Contributor Author

TFTR!

bors r=knz

typeorm runtime:

Before:

--- PASS: typeorm (1851.56s)
--- PASS: typeorm (1866.40s)
--- PASS: typeorm (1927.17s)

After:

--- PASS: typeorm (1599.05s)
--- PASS: typeorm (1632.52s)
--- PASS: typeorm (1640.89s)

@craig
Copy link
Contributor

craig bot commented Aug 31, 2020

Build succeeded:

@craig craig bot merged commit 410616f into cockroachdb:master Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz left review comments

@tbg tbg Awaiting requested review from tbg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajwerner @cockroach-teamcity @knz